Skip to content

Dispose the certificate chain elements with the chain #62531

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Dispose the certificate chain elements with the chain #62531

wants to merge 4 commits into from
+46 −18

Conversation

jashook
Copy link

@jashook jashook commented Jul 1, 2025 •
edited
Loading

Dispose the certificate chain elements within the chain

This pr is going to fix a series of native memory leaks we have seen due to leaking certificates on the chain at Roblox. (fingers crossed)

  • You've read the Contributor Guide and Code of Conduct.
  • You've included unit or integration tests for your change, where applicable.
  • You've included inline docs for your change, where applicable.
  • There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Summary of the changes (Less than 80 chars)

Description

{Detail}

Fixes #{bug number} (in this specific format)

@github-actions github-actions bot added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Jul 1, 2025
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 1, 2025
@jashook jashook marked this pull request as ready for review July 2, 2025 00:42
@jashook jashook requested a review from halter73 as a code owner July 2, 2025 00:42
@dotnet-policy-service dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jul 9, 2025
@jashook
Copy link
Author

jashook commented Jul 9, 2025

Gentle ping @halter73

@jashook
Copy link
Author

jashook commented Jul 11, 2025

cc @janvorli

@janvorli
Copy link
Member

cc: @rzikm

rzikm
rzikm approved these changes Jul 11, 2025
View reviewed changes
Copy link
Member

@rzikm rzikm left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this is in line with what we do in SslStream

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Security/src/System/Net/Security/SslStream.Protocol.cs#L1148-L1163

Note that this PR doesn't fix a "leak" per se. The cert instances will be eventually collected by GC and finalization will ensure the native resources are released. However, explicitly disposing the certs is definitely an improvement.

@rzikm
Copy link
Member

rzikm commented Jul 11, 2025 •
edited
Loading

You may also want to dispose certificate at this callsite

https://github.com/jashook/aspnetcore/blob/cf7235c3a0b8fc5a841dd4838fdea4975b5d3557/src/Shared/CertificateGeneration/UnixCertificateManager.cs#L111

@jashook
Copy link
Author

jashook commented Jul 12, 2025 •
edited
Loading

Note that this PR doesn't fix a "leak" per se. The cert instances will be eventually collected by GC and finalization will ensure the native resources are released

Yes, and no, for us the rate at which we do tls handshakes outpaces the rate of gc. Which leads to unbounded memory growth, until the gc collects aggressively, at which the application will health check and die.

As in, yes you are correct this is not a native leak from the runtime, but it is effectively a managed leak with native resources which leads to the application degrading and restarting.

cc @leculver

@jashook
Copy link
Author

jashook commented Jul 12, 2025

Will address the comment. Can we take this into net8?

@jashook
Copy link
Author

jashook commented Jul 12, 2025 •
edited
Loading

You may also want to dispose certificate at this callsite

https://github.com/jashook/aspnetcore/blob/cf7235c3a0b8fc5a841dd4838fdea4975b5d3557/src/Shared/CertificateGeneration/UnixCertificateManager.cs#L111

😅 lol nice catch...

@jashook
Copy link
Author

jashook commented Jul 12, 2025

Seems like this can be merged. cc @halter73

@jashook
Copy link
Author

jashook commented Jul 12, 2025

:shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiagonapoli tiagonapoli tiagonapoli approved these changes

@rzikm rzikm rzikm approved these changes

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

Assignees
No one assigned
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer community-contribution Indicates that the PR has been added by a community member pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jashook @janvorli @rzikm @tiagonapoli